Add support for HTTPOption by ReToCode · Pull Request #437 · knative-extensions/net-gateway-api

ReToCode · 2023-01-18T09:53:42Z

This PR introduces support for HTTPOption (introduced in knative/networking#415).

Changes

🎁 Add support for HTTPOption
🎁 Introduces a new configuration to specify the name of the http-listener for the gateways
🧹 Cleanup some redundancies in test code
🧹 Use new functions where deprecations were used

The implementation is based on the gateway-api example for redirects: https://github.com/kubernetes-sigs/gateway-api/blob/main/examples/standard/http-redirect.yaml. It was necessary to restructure some code, as two HTTPRoute objects need to be created, each referencing only a specific section in the gateway using sectionName. We should further discuss if we should use that approach also for the "normal" use-case (as we currently bind to all the sections in the gateway only differentiated by the hostname in the TLS case). Also The configuration key visibility seems now a bit off, maybe we can find a better name?

A full example of how the resources look like can be found here: https://gist.github.com/ReToCode/2e4d2b3223752c6f380348ef027c55b3

/kind cleanup
/kind enhancement

Fixes #130

knative-prow · 2023-01-18T09:53:44Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

ReToCode · 2023-01-18T09:53:54Z

/test all

ReToCode · 2023-01-18T09:54:46Z

/cc @nak3 , @dprotaso could you please look over this. I'm a bit unsure if this is the way to go.

codecov · 2023-01-18T09:57:07Z

Codecov Report

Attention: Patch coverage is 79.20000% with 26 lines in your changes missing coverage. Please review.

Project coverage is 79.02%. Comparing base (217f929) to head (5256377).
Report is 71 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/reconciler/ingress/reconcile_resources.go	52.50%	15 Missing and 4 partials ⚠️
pkg/reconciler/ingress/resources/httproute.go	90.41%	3 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #437      +/-   ##
==========================================
- Coverage   79.12%   79.02%   -0.11%     
==========================================
  Files          17       17              
  Lines        1260     1373     +113     
==========================================
+ Hits          997     1085      +88     
- Misses        227      245      +18     
- Partials       36       43       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ReToCode · 2023-01-18T13:26:32Z

Also not really sure about this change: 663d28b. Either unit tests complain about header mismatch (https://github.com/knative-sandbox/net-gateway-api/actions/runs/3949133837/jobs/6759963500#step:7:173) or golangci-lint complains about not preallocating the slice (https://github.com/knative-sandbox/net-gateway-api/actions/runs/3947688057/jobs/6756800214).

ReToCode · 2023-01-18T13:43:47Z

/retest

ReToCode · 2023-01-18T14:00:42Z

/test integration-tests-istio

nak3 · 2023-01-19T10:54:11Z

Thank you so much, Reto!

It was necessary to restructure some code, as two HTTPRoute objects need to be created, each referencing only a specific section in the gateway using sectionName.

I wondered that upstream gateway-api can improve the two HTTPRoute objects for HTTPS redirection (I believe it is too redundant) and actually there are some on-going discussion:

Once the upstream's latest example httproute-redirect-https.yaml (currently invalid!) work, we can simply implement this feature, can't we?

ReToCode · 2023-01-19T13:09:48Z

Interesting, haven't seen that. You are right about the verbosity, I agree there. But the discussion seems also unsure on how to resolve this. Seems like the current stable state is also the last discussed state (kubernetes-sigs/gateway-api#1185 (comment))?
Also I haven't found an issue that is related to the example you linked, so not sure if there is work going in that direction. But I'd be fine to hold this feature here until this is more clear.

nak3 · 2023-01-19T13:29:27Z

Also I haven't found an issue that is related to the example you linked

Hmm... As far as I tested, it keeps redirecting to HTTPS as explained in kubernetes-sigs/gateway-api#1185 and never reached out to the backendRefs(example-svc). To solve it, it was necessary to create two HTTPRoutes.

Anyway, kubernetes-sigs/gateway-api#1185 was tagged to 0.7.0 milestone so maybe it is better to wait a little bit for the upstream.

github-actions · 2023-08-07T01:31:19Z

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.

nak3 · 2023-08-07T02:01:43Z

/remove-lifecycle stale

ReToCode · 2023-08-08T11:58:46Z

As it does not look like upstream gw-api is going to change this behaviour soon (the issue was moved out of GA) and the current redirect behaviour is perceived as stable, let's give the current approach another go.

PTAL: @nak3 , @dprotaso

ReToCode · 2023-08-08T12:21:28Z

/retest

ReToCode · 2023-08-30T08:20:48Z

/assign @nak3
/assign @dprotaso

ReToCode · 2024-07-03T05:54:31Z

@dprotaso @skonto rebased, PTAL.

ReToCode · 2024-07-03T11:19:12Z

/hold

ReToCode · 2024-07-03T13:19:54Z

HTTP-Option test does not yet work on envoy gateway because of envoyproxy/gateway#2149.

/unhold

arkodg · 2024-08-09T01:34:56Z

HTTP-Option test does not yet work on envoy gateway because of envoyproxy/gateway#2149.

/unhold

hey @ReToCode thanks for flagging this, the issue has been fixed, can we retry the test for Envoy Gateway ?

ReToCode · 2024-08-09T05:35:05Z

@arkodg looks great. Thanks for the fix and the ping 👍

skonto · 2024-09-27T11:59:54Z

pkg/reconciler/ingress/ingress.go


-		if isHTTPRouteReady(httproute) {
+		// For now, we only generate the redirected HTTPRoute for external visibility,
+		// because currently we do not (yet) support HTTPs for cluster-local domains in net-gateway-api.


Do we support that for internal encryption and Kourier? 🤔
What is the status for the rest of the ingress implementations?

yes Kourier supports it, net-istio does not need it and net-contour work was started but never finished.
But as this is quite niche and not in the conformance spec, I think we are fine without it here.

skonto · 2024-10-23T13:24:26Z

@dprotaso gentle ping, should we merge this?

knative-prow-robot · 2024-12-22T15:20:59Z

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

github-actions · 2025-03-23T01:32:58Z

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.

ReToCode force-pushed the http-option branch from 10c38a5 to 73ce526 Compare January 18, 2023 09:55

ReToCode mentioned this pull request Feb 10, 2023

Add TestHTTPOption for gateway-api #130

Open

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 18, 2023

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 7, 2023

knative-prow bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 7, 2023

ReToCode force-pushed the http-option branch from 9a9faf8 to 2422ce5 Compare August 8, 2023 11:52

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2023

ReToCode marked this pull request as ready for review August 8, 2023 12:05

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 8, 2023

knative-prow bot requested review from dprotaso and upodroid August 8, 2023 12:05

ReToCode force-pushed the http-option branch from e54d3eb to 8ecfd4d Compare July 3, 2024 05:53

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 3, 2024

ReToCode added 2 commits July 3, 2024 08:04

Fix lint warnings

6361d90

Fix e2e tests

44ad75b

knative-extensions deleted a comment from knative-prow bot Jul 3, 2024

Add more test cases

9d5ffcd

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 3, 2024

ReToCode mentioned this pull request Jul 3, 2024

Multiple Reference Grant for specific service in the same namespace didn't work as expected. envoyproxy/gateway#2149

Closed

ReToCode added 2 commits July 3, 2024 15:15

Do not run httpoption for envoy-gateway

4a606af

Add another test case

f4036eb

ReToCode force-pushed the http-option branch from b19449d to f4036eb Compare July 3, 2024 13:19

knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 3, 2024

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 3, 2024

knative-extensions deleted a comment from knative-prow bot Jul 3, 2024

Re-enable http-option test for envoy-gateway

5256377

skonto reviewed Sep 27, 2024

View reviewed changes

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 22, 2024

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 23, 2025

github-actions bot closed this Apr 22, 2025

Conversation

ReToCode commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

knative-prow bot commented Jan 18, 2023

Uh oh!

ReToCode commented Jan 18, 2023

Uh oh!

ReToCode commented Jan 18, 2023

Uh oh!

codecov bot commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ReToCode commented Jan 18, 2023

Uh oh!

ReToCode commented Jan 18, 2023

Uh oh!

ReToCode commented Jan 18, 2023

Uh oh!

nak3 commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ReToCode commented Jan 19, 2023

Uh oh!

nak3 commented Jan 19, 2023

Uh oh!

github-actions bot commented Aug 7, 2023

Uh oh!

nak3 commented Aug 7, 2023

Uh oh!

ReToCode commented Aug 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ReToCode commented Aug 8, 2023

Uh oh!

ReToCode commented Aug 30, 2023

Uh oh!

ReToCode commented Jul 3, 2024

Uh oh!

ReToCode commented Jul 3, 2024

Uh oh!

ReToCode commented Jul 3, 2024

Uh oh!

arkodg commented Aug 9, 2024

Uh oh!

ReToCode commented Aug 9, 2024

Uh oh!

skonto Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ReToCode Sep 30, 2024

Choose a reason for hiding this comment

Uh oh!

skonto commented Oct 23, 2024

Uh oh!

knative-prow-robot commented Dec 22, 2024

Uh oh!

github-actions bot commented Mar 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ReToCode commented Jan 18, 2023 •

edited

Loading

codecov bot commented Jan 18, 2023 •

edited

Loading

nak3 commented Jan 19, 2023 •

edited

Loading

ReToCode commented Aug 8, 2023 •

edited

Loading

skonto Sep 27, 2024 •

edited

Loading